Skip to content

Text-Based Quest#10

Open
wladimir-schneider wants to merge 3 commits intodemologin:mainfrom
wladimir-schneider:main
Open

Text-Based Quest#10
wladimir-schneider wants to merge 3 commits intodemologin:mainfrom
wladimir-schneider:main

Conversation

@wladimir-schneider
Copy link

Text quest engine with persistent quest progress per user
In-memory database for learning purposes
User authentication (login / logout)
Admin role with user management (delete)

Default admin account for testing
Username: admin
Password: admin

Admins and users can create and play quests
Quest progress is preserved between logins
Multi-language support: EN / DE / RU
Event logging via Tomcat 10 Java Logger
Unit tests for core functionality

Can be built with Java JDK 21 using the Maven build tool and run with Tomcat 10.

@demologin
Copy link
Owner

Общий вывод по проекту

Проект представляет собой добротную базу для веб-приложения на сервлетах. Видно уверенное владение инструментами тестирования (JUnit 5, Mockito) и понимание жизненного цикла веб-приложения (использование ServletContextListener, init методов). Код структурирован и легко читается.

Основные рекомендации:

Безопасность: Реализовать хеширование паролей и защитить эндпоинты через фильтры (Filters), а не ручными проверками в каждом сервлете (это мы еще изучим)

Архитектура: Внедрить Service-слой между контроллерами (Servlets) и DAO для размещения там бизнес-логики игрового процесса (после hibernate)

Валидация: Добавить более строгие проверки входных данных (null, пустые строки) для предотвращения сбоев в работе приложения (валидация тоже еще будет)

Итоговая оценка: A

private final Map<String, Quest> allQuests = Collections.synchronizedMap(new HashMap<>());

public InMemoryUserDao() {
User admin = new User();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование 'ConcurrentHashMap' для хранения пользователей — это отличная практика для обеспечения потокобезопасности в InMemory хранилище.

public Quest getQuest(String questId) {
return allQuests.get(questId);
}
public boolean add(User u) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод 'add' не проверяет входной объект на null перед вызовом 'u.getUsername()', что может привести к NullPointerException.

String username = req.getParameter("username");
String password = req.getParameter("password");
log.info("Login attempt for user: " + username);
if (username == null || password == null) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пароли хранятся и сравниваются в открытом виде. Рекомендуется использовать хеширование (например, BCrypt) для обеспечения безопасности данных пользователей.

java.util.Optional<User> u = dao.authenticate(username.trim(), password);
if (u.isEmpty()) {
req.setAttribute("error", "Invalid credentials");
log.warning("Invalid credentials for username: " + username);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Логирование паролей или факта ошибки входа с указанием конкретного имени пользователя может помочь злоумышленникам при подборе. Используйте более обобщенные сообщения.

}
log.info("Registering new user: " + username);
User u = new User();
u.setUsername(username.trim());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вызов 'username.trim()' выполняется без предварительной проверки на null, что вызовет NPE при отсутствии параметра в запросе.


@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Object o = req.getSession().getAttribute("currentUser");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для управления состоянием квеста (progress) вместо строк рекомендуется использовать Enum. Это исключит опечатки в логике переходов.

dao = (InMemoryUserDao) getServletContext().getAttribute("userDao");
}

private boolean isAdmin(HttpServletRequest req) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверка роли пользователя осуществляется напрямую в методе контроллера. Целесообразно вынести авторизацию в Servlet Filter для централизованного управления доступом.

private Role role = Role.USER;
private String progress = "start";

public User() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поле 'id' инициализируется через 'UUID.randomUUID()'. Это хорошее решение для обеспечения уникальности в распределенных системах.

public User() {
}

public String getId() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Рекомендуется сделать поля 'username' и 'password' финальными, если они не предполагают изменения после регистрации, для обеспечения иммутабельности.

public class AppContextListener implements ServletContextListener {
private static final Logger log = Logger.getLogger(AppContextListener.class.getName());
@Override
public void contextInitialized(ServletContextEvent sce) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Инициализация DAO в 'contextInitialized' — правильный подход для внедрения зависимостей (Dependency Injection) через ServletContext.


@BeforeEach
void setUp() throws ServletException {
servlet = new AdminServlet();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тест вручную вызывает 'init(servletConfig)'. При использовании Mockito лучше настраивать моки так, чтобы сервлет получал зависимости через конструктор или сеттеры.


public class InMemoryUserDaoTest {
@Test
public void addAndFindUser() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тестовый метод объединяет несколько проверок (создание, поиск, удаление). Рекомендуется разделять тесты: один метод — один проверяемый сценарий.

this.creator = creator;
}

// Backwards-compatible constructor: from linear list of step descriptions
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В конструкторе класса 'Quest' стоит добавить валидацию на пустой список шагов (steps), чтобы избежать некорректного состояния объекта.

return true;
}
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование 'java.util.Optional' для поиска пользователя — отличный пример современного Java-стиля, помогающий избежать проверок на null у вызывающей стороны.

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Object o = req.getSession().getAttribute("currentUser");
if (!(o instanceof User u)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Применение 'instanceof' с паттерн-матчингом (Java 16+) — это современно и чисто. Хорошая работа с синтаксисом.

private static final Logger log = Logger.getLogger(LogoutServlet.class.getName());
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
req.getSession().invalidate();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Инвалидация сессии при выходе — критически важная деталь для безопасности веб-приложения. Реализовано верно.

}
log.info("Login successful for user: " + username);
req.getSession().setAttribute("currentUser", u.get());
resp.sendRedirect(req.getContextPath() + "/game");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Жесткое кодирование путей для редиректа ('/game') затрудняет поддержку. Стоит использовать константы или конфигурационный файл.

import java.util.*;

public class InMemoryUserDao {
private final Map<String, User> users = Collections.synchronizedMap(new LinkedHashMap<>());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Класс 'InMemoryUserDao' реализует интерфейс (предположительно), но стоит явно выделить интерфейс 'UserDao' для соблюдения принципа инверсии зависимостей (DIP).



@Test
void doPost_successfulRegistration() throws ServletException, IOException {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тест на регистрацию проверяет сразу и DAO, и состояние сессии, и редирект. Это делает тест хрупким. Желательно проверять только поведение сервлета.


public void setUsername(String username) {
this.username = username;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод 'equals' и 'hashCode' должны быть переопределены для корректной работы объекта в коллекциях, особенно при использовании 'id' на базе UUID.

req.setAttribute("users", users);
req.getRequestDispatcher("/admin.jsp").forward(req, resp);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

При удалении пользователя не выполняется проверка, не пытается ли администратор удалить самого себя. Стоит добавить такую бизнес-проверку.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants